[Export CSV] Export data stored in new proto-based format #1917
Conversation
…to gino-m/1779/export-csv-from-pb-data
…to gino-m/1779/export-csv-from-pb-data
…to gino-m/1779/export-csv-from-pb-data
…to gino-m/1779/export-csv-from-pb-data
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…to gino-m/1779/export-csv-from-pb-data
functions/src/export-csv.ts
Outdated
| response: Pb.TaskData.IDateTimeResponse | ||
| ): string | null { | ||
| const seconds = response.dateTime?.seconds; | ||
| if (seconds === undefined || seconds === null) { |
There was a problem hiding this comment.
What did you mean here?
There was a problem hiding this comment.
Sorry, I should have been more clear! There's a funny quirk language feature that allows you to check for both null and undefined with non-strict equality like this:
https://stackoverflow.com/a/16607837
I personally think that it's better to not explicitly check for both null and undefined unless the specifications of the object explicitly define null and undefined to represent different states (which is the 1% case), and opt for simpler, cleaner code to help JS hide it's mistake of putting in TWO nulls in its language ahah.
| @@ -40,4 +39,7 @@ message AuditInfo { | |||
|
|
|||
| // URL of the user's profile picture. | |||
| string photo_url = 5; | |||
|
|
|||
| // The user's email address. | |||
| string email_address = 6; | |||
There was a problem hiding this comment.
I should note that we will need to fix the app client to address this, since we were storing and querying by user_id, which may constitute a large/breaking change...
Can you file a bug on the Android side and I can quickly get this fixed?
There was a problem hiding this comment.
Querying for what, exactly? Ad hoc LOIs? If so, wouldn't that continue to work, since this is just an additive change?
There was a problem hiding this comment.
Ooh... I must have gotten the Survey's ACL field confused with AuditInfo. I think we should be ok! We'll just want to make sure we're clear which field is meant to be the primary key etc.
There was a problem hiding this comment.
@sufyanAbbasi All comments resolved, modulo two follow-up questions. PTAL?
| @@ -40,4 +39,7 @@ message AuditInfo { | |||
|
|
|||
| // URL of the user's profile picture. | |||
| string photo_url = 5; | |||
|
|
|||
| // The user's email address. | |||
| string email_address = 6; | |||
There was a problem hiding this comment.
Querying for what, exactly? Ad hoc LOIs? If so, wouldn't that continue to work, since this is just an additive change?
functions/src/export-csv.ts
Outdated
| response: Pb.TaskData.IDateTimeResponse | ||
| ): string | null { | ||
| const seconds = response.dateTime?.seconds; | ||
| if (seconds === undefined || seconds === null) { |
There was a problem hiding this comment.
Sorry, I should have been more clear! There's a funny quirk language feature that allows you to check for both null and undefined with non-strict equality like this:
https://stackoverflow.com/a/16607837
I personally think that it's better to not explicitly check for both null and undefined unless the specifications of the object explicitly define null and undefined to represent different states (which is the 1% case), and opt for simpler, cleaner code to help JS hide it's mistake of putting in TWO nulls in its language ahah.
| @@ -40,4 +39,7 @@ message AuditInfo { | |||
|
|
|||
| // URL of the user's profile picture. | |||
| string photo_url = 5; | |||
|
|
|||
| // The user's email address. | |||
| string email_address = 6; | |||
There was a problem hiding this comment.
Ooh... I must have gotten the Survey's ACL field confused with AuditInfo. I think we should be ok! We'll just want to make sure we're clear which field is meant to be the primary key etc.
…com/google/ground-platform into gino-m/1779/export-csv-from-pb-data
Towards #1779.
One merged, I'll send a separate PR to read the
Surveymetadata in the new proto format when exporting.